fix: default the Jackson max-decompressed-size to unlimited - #3515
Conversation
Motivation: A bounded default could reject a payload an existing system legitimately exchanges, so a patch release carrying the 256 MiB default from apache#3491 could break running systems on upgrade. The bound should be opt-in, matching the change made to pekko.serialization.max-decompressed-size in apache#3502. Modification: Default pekko.serialization.jackson.compression.max-decompressed-size (and the jackson3 equivalent) to -1, meaning no limit and matching the behaviour of releases before apache#3491. A negative maximum skips the gzip size check and the LZ4 declared-size check; a negative declared LZ4 size is still rejected, since it is malformed regardless of the limit. Config's getBytes refuses negative numbers, so the setting is read as a plain long first and as a memory size only when that is not a negative number. Result: Jackson payload decompression is unbounded by default; configuring a size such as 256 MiB bounds it. Tests: - sbt "serialization-jackson/testOnly org.apache.pekko.serialization.jackson.*" - 122 passed - sbt "serialization-jackson3/testOnly org.apache.pekko.serialization.jackson3.*" - 120 passed - sbt "serialization-jackson/scalafmtCheckAll" "serialization-jackson3/scalafmtCheckAll" - clean - sbt "serialization-jackson/mimaReportBinaryIssues" - no issues References: Refs apache#3491, Refs apache#3502
|
I want to backport #3491 with this PR's modification as part of the 1.7.1 release. The limit becomes opt-in as a result. |
| # The default of -1 applies no limit, preserving the behaviour of earlier | ||
| # releases; set a size such as `256 MiB` to bound decompression, choosing a | ||
| # value larger than any payload the system legitimately exchanges. | ||
| max-decompressed-size = -1 |
There was a problem hiding this comment.
Pekko overall is a bit inconsistent in this, some settings use -1 for unlimited, some settings use
max-received-message-size = unlimited
In general I find such keys more clear than using magic numbers like -1 or 0 that change the meaning of the setting.
Maybe something we can address in 2.0
There was a problem hiding this comment.
Good point. Changed so that both spellings work: the default is now written as unlimited, and a negative number such as -1 is also accepted (matching the neighbouring read.max-document-length / read.max-token-count convention). The same dual handling is applied to pekko.serialization.max-decompressed-size in #3502 so the two sibling settings stay consistent.
Motivation: Review on apache#3515 noted that Pekko is inconsistent about unlimited spellings and an explicit keyword is clearer than a magic number, while the neighbouring read.max-document-length and read.max-token-count settings use -1. Accept both. Modification: The setting reads "unlimited" or any negative number as no limit; the reference.conf default is written as `unlimited`. Applied to both serialization-jackson and serialization-jackson3, with a test each for the keyword. Result: `max-decompressed-size = unlimited` and `= -1` both disable the bound. Tests: - sbt "serialization-jackson/testOnly org.apache.pekko.serialization.jackson.*" - 123 passed - sbt "serialization-jackson3/testOnly org.apache.pekko.serialization.jackson3.*" - 121 passed - sbt "serialization-jackson/scalafmtCheckAll" "serialization-jackson3/scalafmtCheckAll" - clean References: Refs apache#3515
Motivation: Review on apache#3515 noted that an explicit keyword is clearer than a magic number. Keep the two sibling settings consistent: accept both spellings here as well. Modification: pekko.serialization.max-decompressed-size reads "unlimited" or any negative number as no limit; the reference.conf default is written as `unlimited`. New tests cover the keyword default and an explicit -1. Result: `max-decompressed-size = unlimited` and `= -1` both disable the bound. Tests: - sbt "actor-tests/testOnly org.apache.pekko.serialization.DecompressionSpec" - 9 passed - sbt "actor/scalafmtCheckAll" "actor-tests/scalafmtCheckAll" - clean References: Refs apache#3515, Refs apache#3502
* fix: bound the size a compressed payload may expand to Motivation: Five serializers gzip their payload and decompress it on the way back in, each with the same unbounded loop: read the whole GZIPInputStream into a ByteArrayOutputStream. gzip expands by up to about three orders of magnitude, so neither the size of the compressed bytes nor the transport's frame limit bounds the buffer the decompressed bytes are read into. Modification: Add Decompression (@internalapi) with a gunzip that stops once the decompressed size passes pekko.serialization.max-decompressed-size (default 256 MiB) and reports it as a NotSerializableException, and route all twelve call sites through it. The Jackson serializers already bound decompression and keep their own pekko.serialization.jackson.compression.max-decompressed-size. Result: An over-expanding payload is rejected as an ordinary serialization failure. No behaviour change for payloads within the limit. * change the default max-decompressed-size to unlimited Motivation: A bounded default could reject a payload an existing cluster legitimately exchanges, so a patch release carrying a 256 MiB default could break running clusters on upgrade. The bound should be opt-in. Modification: Default pekko.serialization.max-decompressed-size to -1, meaning no limit and matching the behaviour of earlier releases. A negative maximum skips the size check in gunzip. Config's getBytes refuses negative numbers, so the setting is read as a plain long first and as a memory size only when that is not a negative number. Result: Decompression is unbounded by default; configuring a size such as 256 MiB bounds it. Tests: - sbt "actor-tests/testOnly org.apache.pekko.serialization.DecompressionSpec" - 8 passed - sbt "cluster/testOnly org.apache.pekko.cluster.protobuf.ClusterMessageSerializerDecompressionSpec" - 4 passed - sbt "distributed-data/testOnly org.apache.pekko.cluster.ddata.protobuf.SerializationSupportDecompressionSpec" - 3 passed - sbt "actor/scalafmtCheckAll" "actor-tests/scalafmtCheckAll" - clean References: Refs #3502 * also accept "unlimited" for max-decompressed-size Motivation: Review on #3515 noted that an explicit keyword is clearer than a magic number. Keep the two sibling settings consistent: accept both spellings here as well. Modification: pekko.serialization.max-decompressed-size reads "unlimited" or any negative number as no limit; the reference.conf default is written as `unlimited`. New tests cover the keyword default and an explicit -1. Result: `max-decompressed-size = unlimited` and `= -1` both disable the bound. Tests: - sbt "actor-tests/testOnly org.apache.pekko.serialization.DecompressionSpec" - 9 passed - sbt "actor/scalafmtCheckAll" "actor-tests/scalafmtCheckAll" - clean References: Refs #3515, Refs #3502
* fix: bound the size a compressed payload may expand to Motivation: Five serializers gzip their payload and decompress it on the way back in, each with the same unbounded loop: read the whole GZIPInputStream into a ByteArrayOutputStream. gzip expands by up to about three orders of magnitude, so neither the size of the compressed bytes nor the transport's frame limit bounds the buffer the decompressed bytes are read into. Modification: Add Decompression (@internalapi) with a gunzip that stops once the decompressed size passes pekko.serialization.max-decompressed-size (default 256 MiB) and reports it as a NotSerializableException, and route all twelve call sites through it. The Jackson serializers already bound decompression and keep their own pekko.serialization.jackson.compression.max-decompressed-size. Result: An over-expanding payload is rejected as an ordinary serialization failure. No behaviour change for payloads within the limit. * change the default max-decompressed-size to unlimited Motivation: A bounded default could reject a payload an existing cluster legitimately exchanges, so a patch release carrying a 256 MiB default could break running clusters on upgrade. The bound should be opt-in. Modification: Default pekko.serialization.max-decompressed-size to -1, meaning no limit and matching the behaviour of earlier releases. A negative maximum skips the size check in gunzip. Config's getBytes refuses negative numbers, so the setting is read as a plain long first and as a memory size only when that is not a negative number. Result: Decompression is unbounded by default; configuring a size such as 256 MiB bounds it. Tests: - sbt "actor-tests/testOnly org.apache.pekko.serialization.DecompressionSpec" - 8 passed - sbt "cluster/testOnly org.apache.pekko.cluster.protobuf.ClusterMessageSerializerDecompressionSpec" - 4 passed - sbt "distributed-data/testOnly org.apache.pekko.cluster.ddata.protobuf.SerializationSupportDecompressionSpec" - 3 passed - sbt "actor/scalafmtCheckAll" "actor-tests/scalafmtCheckAll" - clean References: Refs #3502 * also accept "unlimited" for max-decompressed-size Motivation: Review on #3515 noted that an explicit keyword is clearer than a magic number. Keep the two sibling settings consistent: accept both spellings here as well. Modification: pekko.serialization.max-decompressed-size reads "unlimited" or any negative number as no limit; the reference.conf default is written as `unlimited`. New tests cover the keyword default and an explicit -1. Result: `max-decompressed-size = unlimited` and `= -1` both disable the bound. Tests: - sbt "actor-tests/testOnly org.apache.pekko.serialization.DecompressionSpec" - 9 passed - sbt "actor/scalafmtCheckAll" "actor-tests/scalafmtCheckAll" - clean References: Refs #3515, Refs #3502
…3491, #3515) (#3523) * fix: bound Jackson payload decompression size (#3491) Motivation: JacksonSerializer.decompress inflated gzip payloads with an unbounded transferTo, and passed the lz4 decompressed length declared on the wire straight to the decompressor as the allocation size. A small, well-formed message could therefore declare (or expand to) an arbitrarily large size and drive an OutOfMemoryError on deserialization. Modification: Add a `compression.max-decompressed-size` setting (default 256 MiB) to the jackson and jackson3 modules. On deserialization the gzip path copies through a bounded loop and the lz4 path rejects a declared length that is negative or over the cap, before allocating. Applies regardless of the `algorithm` setting, since decompression is chosen by the payload's magic bytes. Result: A payload that would decompress beyond the cap is rejected with an IllegalArgumentException instead of exhausting the heap. Tests: - sbt "serialization-jackson/testOnly *JacksonJsonSerializerSpec" "serialization-jackson3/testOnly *JacksonJsonSerializerSpec" - 71 passed each, incl. new gzip/lz4 cap tests - sbt "serialization-jackson/mimaReportBinaryIssues" - no issues (changed symbols are @InternalApi/private) - sbt scalafmt for changed main and test sources References: None - found while reviewing the draft threat model in #3478 * fix: default the Jackson max-decompressed-size to unlimited (#3515) * fix: default the Jackson max-decompressed-size to unlimited Motivation: A bounded default could reject a payload an existing system legitimately exchanges, so a patch release carrying the 256 MiB default from #3491 could break running systems on upgrade. The bound should be opt-in, matching the change made to pekko.serialization.max-decompressed-size in #3502. Modification: Default pekko.serialization.jackson.compression.max-decompressed-size (and the jackson3 equivalent) to -1, meaning no limit and matching the behaviour of releases before #3491. A negative maximum skips the gzip size check and the LZ4 declared-size check; a negative declared LZ4 size is still rejected, since it is malformed regardless of the limit. Config's getBytes refuses negative numbers, so the setting is read as a plain long first and as a memory size only when that is not a negative number. Result: Jackson payload decompression is unbounded by default; configuring a size such as 256 MiB bounds it. Tests: - sbt "serialization-jackson/testOnly org.apache.pekko.serialization.jackson.*" - 122 passed - sbt "serialization-jackson3/testOnly org.apache.pekko.serialization.jackson3.*" - 120 passed - sbt "serialization-jackson/scalafmtCheckAll" "serialization-jackson3/scalafmtCheckAll" - clean - sbt "serialization-jackson/mimaReportBinaryIssues" - no issues References: Refs #3491, Refs #3502 * also accept "unlimited" for max-decompressed-size Motivation: Review on #3515 noted that Pekko is inconsistent about unlimited spellings and an explicit keyword is clearer than a magic number, while the neighbouring read.max-document-length and read.max-token-count settings use -1. Accept both. Modification: The setting reads "unlimited" or any negative number as no limit; the reference.conf default is written as `unlimited`. Applied to both serialization-jackson and serialization-jackson3, with a test each for the keyword. Result: `max-decompressed-size = unlimited` and `= -1` both disable the bound. Tests: - sbt "serialization-jackson/testOnly org.apache.pekko.serialization.jackson.*" - 123 passed - sbt "serialization-jackson3/testOnly org.apache.pekko.serialization.jackson3.*" - 121 passed - sbt "serialization-jackson/scalafmtCheckAll" "serialization-jackson3/scalafmtCheckAll" - clean References: Refs #3515
Motivation
#3491 bounded Jackson payload decompression with
pekko.serialization.jackson.compression.max-decompressed-size, defaulting to 256 MiB. Abounded default could reject a payload an existing system legitimately exchanges, so a
patch release carrying it could break running systems on upgrade, and there is no default
that is provably above every deployment's largest payload. The bound should be opt-in,
matching the change made to
pekko.serialization.max-decompressed-sizein #3502.Modification
Default the setting to
unlimited, meaning no limit and matching the behaviour ofreleases before #3491, in both
serialization-jacksonandserialization-jackson3. Anegative number such as
-1also means unlimited — the convention of the neighbouringread.max-document-lengthandread.max-token-countsettings — per review, so bothspellings are accepted. Operators who want the protection set a size such as
256 MiB,larger than anything their system legitimately sends.
An unlimited maximum skips the gzip size check and the LZ4 declared-size check. A negative
declared LZ4 size is still rejected — it is malformed regardless of the limit — and now
with a message saying that, rather than one claiming it exceeds the maximum.
Config's
getBytesrefuses both spellings, so the setting is read as a string first andas a memory size only when it is neither
unlimitednor a negative number.Result
Jackson payload decompression is unbounded by default; configuring a size bounds it. A
configured limit behaves exactly as before.
Tests
Three new tests per module (so each runs under both the JSON and CBOR serializers):
apply no gzip decompression limit when max-decompressed-size is -1apply no lz4 decompression limit when max-decompressed-size is -1apply no decompression limit when max-decompressed-size is unlimitedThe
-1tests were checked to discriminate by reverting the production change andre-running: each then fails with
ConfigException$BadValue: Attempt to construct memory size with negative number: -1, which is also what an operator configuring-1on thecurrent code would get at serializer construction;
unlimitedfails the same way as aBadValueon the keyword.sbt "serialization-jackson/testOnly org.apache.pekko.serialization.jackson.*"— passedsbt "serialization-jackson3/testOnly org.apache.pekko.serialization.jackson3.*"— passedsbt "serialization-jackson/scalafmtCheckAll" "serialization-jackson3/scalafmtCheckAll"— cleansbt "serialization-jackson/mimaReportBinaryIssues"— no issues (serialization-jackson3disables MimaPlugin)
References
Refs #3491, Refs #3502